Skip to content

Conversation

@nilptr
Copy link
Contributor

@nilptr nilptr commented Dec 8, 2024

Summary

close #5091

  • introduce a hybrid filesystem to dispatch fs read between Javascript side or Rust.
  • use inputFileSystem to decide one file should node.js FS or rust FS.

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

@netlify
Copy link

netlify bot commented Dec 8, 2024

Deploy Preview for rspack canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 2f93d9e
🔍 Latest deploy log https://app.netlify.com/projects/rspack/deploys/68414a30965d740008113142

@nilptr nilptr force-pushed the nilptr/feat/input-file-system branch from e46d8a3 to 3162187 Compare December 8, 2024 06:05
@nilptr nilptr changed the title consume js inputFileSystem on the rust side Draft: consume js inputFileSystem on the rust side Dec 8, 2024
@nilptr nilptr force-pushed the nilptr/feat/input-file-system branch 2 times, most recently from 086d822 to 7a4d76e Compare December 9, 2024 15:11
@nilptr
Copy link
Contributor Author

nilptr commented Dec 9, 2024

Hi guys, I have been struggling to investigate the CI "Test Node 18" failure for 2 days. I feel I really need your help.

I believe my implementation can pass all the tests if the tests are run separately.
However, the tests will always be stuck in the config/builtin-swc-loader/swc-plugin case, when they are run concurrently.

My findings so far are that it's stuck in


even if the output is the same.

@hardfist
Copy link
Contributor

hardfist commented Dec 10, 2024

thanks it maybe related to performance regression, block_on fs binding will cause huge performance regression

I think the biggest blocker now is rspack_resolver doesn't use async_fs, which cause it has to block_on on fs binging call, which will introduce huge performance regression rstackjs/rspack-resolver#34.

but we can still merge this pr first if we don't enable input_fs binding by default and when we solve the rspack_resolver async_fs problem, we can enable inputfs binding

@nilptr
Copy link
Contributor Author

nilptr commented Dec 10, 2024

yeah, I also suspect the performance regression, but cannot prove it.

I'm afraid we cannot merge this PR now, because compiler.inputFileSystem has been set by default.

function createCompiler(userOptions: RspackOptions): Compiler {
const options = getNormalizedRspackOptions(userOptions);
applyRspackOptionsBaseDefaults(options);
assert(!isNil(options.context));
const compiler = new Compiler(options.context, options);
new NodeEnvironmentPlugin({
infrastructureLogging: options.infrastructureLogging
}).apply(compiler);

and

const inputFileSystem: InputFileSystem = new CachedInputFileSystem(
fs,
60000
);
compiler.inputFileSystem = inputFileSystem;

if you feel the refactors in this pr are needed, I can create a new pr and cherry pick the refactor commits.

@hardfist
Copy link
Contributor

hardfist commented Dec 10, 2024

yeah, I also suspect the performance regression, but cannot prove it.

I actually tried it before and revert cause huge performance regression introduced, you can test it in project with large modules like https://github.com/hardfist/performance-compare-ng/tree/master/apps/10000

@hardfist
Copy link
Contributor

hardfist commented Dec 10, 2024

I'm afraid we cannot merge this PR now, because compiler.inputFileSystem has been set by default.

that's easy, you can just ignore the inputFileSystem in the rust side, it's actually current's behavior

@nilptr nilptr mentioned this pull request Dec 10, 2024
2 tasks
@nilptr
Copy link
Contributor Author

nilptr commented Dec 10, 2024

I disabled inputFileSystem on the js side in this PR.

I also created another branch and cherry-picked the refactor commits. maybe #8654 is exactly what you expect.

@hardfist
Copy link
Contributor

I disabled inputFileSystem on the js side in this PR.

I also created another branch and cherry-picked the refactor commits. maybe #8654 is exactly what you expect.

yes, good job!

@nilptr nilptr closed this Dec 11, 2024
@hardfist hardfist reopened this Apr 7, 2025
@hardfist hardfist requested a review from stormslowly April 7, 2025 08:22
@nilptr nilptr force-pushed the nilptr/feat/input-file-system branch from e4a3ede to c588351 Compare April 13, 2025 16:20
@codspeed-hq
Copy link

codspeed-hq bot commented Apr 13, 2025

CodSpeed Performance Report

Merging #8643 will not alter performance

Comparing nilptr:nilptr/feat/input-file-system (2f93d9e) with main (e32b7ed)

Summary

✅ 12 untouched benchmarks

@nilptr nilptr force-pushed the nilptr/feat/input-file-system branch from 40336e8 to f5e3b9b Compare April 20, 2025 16:44
@hardfist hardfist force-pushed the nilptr/feat/input-file-system branch from b04fe0c to ae04347 Compare April 30, 2025 04:11
@watsonhaw5566
Copy link

watsonhaw5566 commented May 13, 2025

@nilptr @stormslowly 你们好,这个 pr 是不是可以支持使用社区的 webpack-virtual-module 的了?

@stormslowly
Copy link
Contributor

@nilptr @stormslowly 你们好,这个 pr 是不是可以支持使用社区的 webpack-virtual-module 的了?

Yes, i will make a canary version; could you please give it try later?

@watsonhaw5566
Copy link

watsonhaw5566 commented May 14, 2025

@nilptr @stormslowly 你们好,这个 pr 是不是可以支持使用社区的 webpack-virtual-module 的了?

Yes, i will make a canary version; could you please give it try later?

ok,thanks. let's talking in Lark.

@stormslowly
Copy link
Contributor

stormslowly commented May 14, 2025

The canary supports virtual module is
@rspack-canary/core: 1.3.10-canary-20f7fc16-20250514042220
@rspack-canary/core: 1.3.10-canary-4a96390f-20250514074254

add below config in you config file

{
  experiments: {
    useInputFileSystem: true
  }
}

you can reference here https://rspack.dev/guide/start/quick-start#install-canary-version for usage.

@watsonhaw5566
Copy link

感谢 @stormslowly @nilptr 兄的付出 ,1.3.10-canary-4a96390f-20250514074254 版本,已经可以正常运行 webpack-virtual-modules

@watsonhaw5566
Copy link

watsonhaw5566 commented May 19, 2025

发现了一个新问题,当使用虚拟模块作为入口文件时,输出的虚拟 js 文件内容为空,并引发了 rspack panic

最小复现 demo 在这里 https://github.com/watsonhaw5566/rspack-virtual-modules-addentry-demo

初步猜想是 compilation.addEntry 当前仅支持真实路径文件,而不支持虚拟文件路径。

@nilptr
Copy link
Contributor Author

nilptr commented May 21, 2025

@watsonhaw5566 there are a few issues with your implementation.

  1. Error: Module not found: Can't resolve 'entry.js' in '/path/to/rspack-virtual-modules-addentry-demo'
    your entry specifier 'entry.js' doesn't start with '/', './' or '../', so the ESM resolution algorithm will search for it in node_modules.
    changing EntryDependency('entry.js') to EntryDependency('./entry.js') or writing the virtual module to node_modules/entry.js can solve this.
  2. Error: build failed with unknown error
    entryLoader will run for every js file, but it's not allowed to add the same entry multiple times. please reconsider your design, don't abuse loader.

@watsonhaw5566
Copy link

watsonhaw5566 commented May 21, 2025

@watsonhaw5566 there are a few issues with your implementation.

  1. Error: Module not found: Can't resolve 'entry.js' in '/path/to/rspack-virtual-modules-addentry-demo'
    your entry specifier 'entry.js' doesn't start with '/', './' or '../', so the ESM resolution algorithm will search for it in node_modules.
    changing EntryDependency('entry.js') to EntryDependency('./entry.js') or writing the virtual module to node_modules/entry.js can solve this.
  2. Error: build failed with unknown error
    entryLoader will run for every js file, but it's not allowed to add the same entry multiple times. please reconsider your design, don't abuse loader.

Thank you suggestions , I'm modify to EntryDependency('./entry.js') , but it will make the Rspack Panic.
But now the question is the compilation.addEntry can't work on loader . #1042

@nilptr
Copy link
Contributor Author

nilptr commented May 21, 2025

But now the question is the compilation.addEntry can't work on loader .

Don't call compilation.addEntry in the loader.
Loaders are meant for transforming source code, not for interacting with the compilation directly. Modifying the compilation like this is something that should be done in a plugin instead.
If you're trying to inject additional modules or manipulate the build process, I recommend moving that logic into a proper plugin.

It looks like your issues are unrelated to the purpose of this PR. To keep the thread focused, please move this discussion to a new thread in the Discussions section if needed.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 4, 2025

📝 Benchmark detail: Open

Name Base (2025-06-04 034dc61) Current Change
10000_big_production-mode_disable-minimize + exec 35.3 s ± 347 ms 36.2 s ± 571 ms +2.36 %
10000_development-mode + exec 1.86 s ± 25 ms 1.84 s ± 162 ms -1.07 %
10000_development-mode_hmr + exec 749 ms ± 26 ms 745 ms ± 22 ms -0.58 %
10000_production-mode + exec 2.28 s ± 41 ms 2.27 s ± 75 ms -0.60 %
10000_production-mode_persistent-cold + exec 2.46 s ± 172 ms 2.41 s ± 24 ms -2.19 %
10000_production-mode_persistent-hot + exec 1.71 s ± 24 ms 1.73 s ± 108 ms +1.19 %
arco-pro_development-mode + exec 1.78 s ± 41 ms 1.77 s ± 31 ms -0.71 %
arco-pro_development-mode_hmr + exec 383 ms ± 0.72 ms 383 ms ± 2.7 ms +0.09 %
arco-pro_production-mode + exec 3.38 s ± 24 ms 3.44 s ± 101 ms +1.78 %
arco-pro_production-mode_generate-package-json-webpack-plugin + exec 3.51 s ± 160 ms 3.57 s ± 93 ms +1.97 %
arco-pro_production-mode_persistent-cold + exec 3.46 s ± 89 ms 3.58 s ± 48 ms +3.45 %
arco-pro_production-mode_persistent-hot + exec 2.12 s ± 51 ms 2.14 s ± 201 ms +1.01 %
arco-pro_production-mode_traverse-chunk-modules + exec 3.47 s ± 316 ms 3.49 s ± 76 ms +0.66 %
large-dyn-imports_development-mode + exec 2.11 s ± 55 ms 2.06 s ± 49 ms -2.30 %
large-dyn-imports_production-mode + exec 2.09 s ± 48 ms 2.06 s ± 67 ms -1.54 %
threejs_development-mode_10x + exec 1.42 s ± 166 ms 1.37 s ± 17 ms -3.10 %
threejs_development-mode_10x_hmr + exec 843 ms ± 3.9 ms 850 ms ± 28 ms +0.74 %
threejs_production-mode_10x + exec 4.87 s ± 37 ms 4.93 s ± 344 ms +1.21 %
threejs_production-mode_10x_persistent-cold + exec 4.94 s ± 53 ms 4.95 s ± 9.9 ms +0.24 %
threejs_production-mode_10x_persistent-hot + exec 4.41 s ± 332 ms 4.36 s ± 55 ms -1.01 %
10000_big_production-mode_disable-minimize + rss memory 9337 MiB ± 487 MiB 9122 MiB ± 29.2 MiB -2.31 %
10000_development-mode + rss memory 650 MiB ± 12 MiB 641 MiB ± 28.7 MiB -1.38 %
10000_development-mode_hmr + rss memory 779 MiB ± 7.61 MiB 769 MiB ± 22.7 MiB -1.21 %
10000_production-mode + rss memory 640 MiB ± 35.1 MiB 655 MiB ± 25.5 MiB +2.43 %
10000_production-mode_persistent-cold + rss memory 758 MiB ± 36 MiB 764 MiB ± 49.5 MiB +0.77 %
10000_production-mode_persistent-hot + rss memory 745 MiB ± 35 MiB 730 MiB ± 91.5 MiB -2.03 %
arco-pro_development-mode + rss memory 582 MiB ± 55.6 MiB 565 MiB ± 59.5 MiB -3.03 %
arco-pro_development-mode_hmr + rss memory 491 MiB ± 45.8 MiB 473 MiB ± 21.2 MiB -3.79 %
arco-pro_production-mode + rss memory 668 MiB ± 49.8 MiB 707 MiB ± 59.2 MiB +5.89 %
arco-pro_production-mode_generate-package-json-webpack-plugin + rss memory 684 MiB ± 52.7 MiB 705 MiB ± 41.9 MiB +3.00 %
arco-pro_production-mode_persistent-cold + rss memory 790 MiB ± 70.3 MiB 768 MiB ± 72.4 MiB -2.81 %
arco-pro_production-mode_persistent-hot + rss memory 644 MiB ± 61.5 MiB 664 MiB ± 98.9 MiB +3.02 %
arco-pro_production-mode_traverse-chunk-modules + rss memory 711 MiB ± 29.1 MiB 700 MiB ± 79.5 MiB -1.54 %
large-dyn-imports_development-mode + rss memory 655 MiB ± 5.08 MiB 660 MiB ± 10.3 MiB +0.81 %
large-dyn-imports_production-mode + rss memory 544 MiB ± 1.56 MiB 542 MiB ± 1.56 MiB -0.27 %
threejs_development-mode_10x + rss memory 587 MiB ± 19.9 MiB 589 MiB ± 30.7 MiB +0.24 %
threejs_development-mode_10x_hmr + rss memory 780 MiB ± 36.1 MiB 779 MiB ± 40.3 MiB -0.21 %
threejs_production-mode_10x + rss memory 866 MiB ± 33.3 MiB 886 MiB ± 41.8 MiB +2.26 %
threejs_production-mode_10x_persistent-cold + rss memory 946 MiB ± 48.5 MiB 952 MiB ± 35.2 MiB +0.62 %
threejs_production-mode_10x_persistent-hot + rss memory 842 MiB ± 39.3 MiB 841 MiB ± 38.6 MiB -0.09 %

@stormslowly stormslowly changed the title Draft: consume js inputFileSystem on the rust side consume js inputFileSystem on the rust side Jun 4, 2025
@stormslowly stormslowly changed the title consume js inputFileSystem on the rust side feat: consume js inputFileSystem on the rust side Jun 4, 2025
@github-actions github-actions bot added the release: feature release: feature related release(mr only) label Jun 4, 2025
@stormslowly stormslowly requested a review from hardfist June 4, 2025 07:45
@chenjiahan chenjiahan requested a review from Copilot June 4, 2025 08:06
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Add support for consuming the JavaScript inputFileSystem on the Rust side by introducing a hybrid filesystem that dispatches reads between Node.js and native Rust FS based on an allowlist.

  • Introduce a new useInputFileSystem experiment option and default it to false.
  • Implement ThreadsafeInputNodeFS in TypeScript and HybridFileSystem in Rust to route file operations.
  • Wire the new experiment through normalization, defaults, compiler binding, and update core API docs and tests.

Reviewed Changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/rspack/src/config/types.ts Define new useInputFileSystem option type
packages/rspack/src/config/normalization.ts Normalize the new experiment setting
packages/rspack/src/config/defaults.ts Set default value for useInputFileSystem
packages/rspack/src/FileSystem.ts Add ThreadsafeInputNodeFS and shared __to_binding_stat helper
packages/rspack/src/Compiler.ts Wire useInputFileSystem into compiler bindings
crates/node_binding/src/lib.rs Pass inputFileSystem through JS-to-Rust binding
crates/node_binding/src/fs_node/hybrid.rs Implement HybridFileSystem in Rust
Comments suppressed due to low confidence (4)

packages/rspack/src/config/types.ts:2631

  • [nitpick] The JSDoc for UseInputFileSystem could be expanded to clarify that providing a RegExp[] acts as an allowlist of paths that should use the Node.js filesystem rather than the Rust filesystem.
/**

packages/rspack/src/Compiler.ts:849

  • Consider adding unit or integration tests for the useInputFileSystem flag to verify that enabling and disabling the hybrid filesystem routing works as intended.
this.inputFileSystem && options.experiments.useInputFileSystem

packages/rspack/src/FileSystem.ts:98

  • Missing imports for util and memoizeFn at the top of this file. For example, add import util from 'util'; and import memoizeFn from its defining module so these calls don’t cause runtime errors.
this.readDir = memoizeFn(() => {

packages/rspack/src/config/defaults.ts:258

  • [nitpick] Minor grammar: consider rephrasing to "Enabling useInputFileSystem introduces additional filesystem overhead, so it is disabled by default."
// Enable `useInputFileSystem` will introduce much more fs overheads,  So disable by default.

@stormslowly stormslowly force-pushed the nilptr/feat/input-file-system branch from da7ec5e to 2f93d9e Compare June 5, 2025 07:41
@hardfist
Copy link
Contributor

hardfist commented Jun 5, 2025

thanks

@stormslowly stormslowly enabled auto-merge (squash) June 10, 2025 05:04
@stormslowly stormslowly merged commit 6719a7f into web-infra-dev:main Jun 10, 2025
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release: feature release: feature related release(mr only)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Failed to resolve xxx in project root error occurred while using inputFileSystem

4 participants